Allow ASan container annotations to activate piecemeal#6211
Allow ASan container annotations to activate piecemeal#6211amyw-msft wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…ted piecemeal. The lib is retained for compatibility with static libraries built with old headers.
| PM_CL="/EHsc /MTd /std:c++latest /permissive- /analyze:only /analyze:autolog-" | ||
| PM_CL="/EHsc /MTd /std:c++latest /permissive- /fp:strict" | ||
| PM_CL="/EHsc /MTd /std:c++latest /permissive-" | ||
| # TRANSITION, we don't use /ALTERNATENAME for Clang (see GH-5224) so we cannot test /D_ANNOTATE_VECTOR without -fsanitize=address |
There was a problem hiding this comment.
This comment was copy-pasted from GH_002030_asan_annotate_vector/env.lst, but as you're crossing with PM_CL="/D_ANNOTATE_STL", I think this comment is inapplicable and should be removed here.
| extern const bool _Asan_string_should_annotate = true; | ||
| extern const bool _Asan_vector_should_annotate = true; | ||
| extern const bool _Asan_optional_should_annotate = true; | ||
| // Retained for compatibility with old headers that add stl_asan.lib to the link. |
There was a problem hiding this comment.
This should use the magic words "TRANSITION, ABI".
| extern __declspec(selectany) const bool _Asan_string_should_annotate = true; | ||
| extern __declspec(selectany) const bool _Asan_vector_should_annotate = true; | ||
| extern __declspec(selectany) const bool _Asan_optional_should_annotate = true; |
There was a problem hiding this comment.
I'm concerned that bundling these 3 definitions into a single object can still lead to scenarios where annotations are partially activated, the linker drags in asan.obj here, and (through the magic of selectany) we end up activating annotations that the user didn't request.
Conversely, I am even more concerned that asan_noop.cpp isn't being changed, and is still a monolithic source file producing a single object file.
Should we break up asan.cpp and asan_noop.cpp into 3 separate files each, prominently commented that they need to remain separate and never fused? stl_asan.lib would still be a single LIB for back-compat containing the 3 OBJs. The linker's behavior operates at an OBJ level of granularity.
| // Use __declspec(selectany) to be compatible with new version that define these | ||
| // variables as __declspec(selectany) in __msvc_sanitizer_annotate_container.hpp. | ||
| // The new method is preferred because previously enabling just string would also | ||
| // enable vector and optional. |
There was a problem hiding this comment.
Adding ", see GH-6186." would be helpful (we try to avoid the need to git blame for programmer-archaeology).
| extern const bool _Asan_vector_should_annotate = true; | ||
| extern const bool _Asan_optional_should_annotate = true; | ||
| // Retained for compatibility with old headers that add stl_asan.lib to the link. | ||
| // Use __declspec(selectany) to be compatible with new version that define these |
There was a problem hiding this comment.
"with new version that define these" isn't grammatical. Should probably be "with the new version that defines these".
|
|
||
| #ifdef _ACTIVATE_STRING_ANNOTATION | ||
| #pragma comment(lib, "stl_asan") | ||
| extern "C" __declspec(selectany) const bool _Asan_string_should_annotate = true; |
There was a problem hiding this comment.
Instead of individually marking these as extern "C", I believe it would be better to move up the extern "C" { scope beginning at right line 148, so that these definitions look more similar to the declarations below. The only stuff in between are pragmas and undefs.
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // REQUIRES: x64 || x86 || arm64 |
There was a problem hiding this comment.
Nitpick: We conventionally put an empty line after the license banner.
github-project-automation
bot
moved this from Initial Review
to Work In Progress
in STL Code Reviews
2 participants
Fixes #6186.
Previously, ASan annotations for
string,vector, andoptionalwere activated by linking withstl_asan.libwhich set each activation variable to true, leading to ODR violations if some TUs were built with annotations inserted but partially activated.This change removes the
stl_asan.libdependency and allows each option to set the activation variable individually.I've retained
stl_asan.liband modified the definitions with__declspec(selectany)to match those in the headers, and also to ensure compatibility if linking with a static library from a previous version of the toolset.The test I've included just ensures the annotations can be targeted individually instead of constructing an ASan error. I did this for simplicity (esp as new annotations are added) while still covering every scenario. I limited it to one disabled out of the set instead of exhaustive validation to not devote too much test time to this.
I've verified the scenario from #6186 locally, as well as ensuring it is backwards compatible with old versions of the headers (meaning, I built the annotated_lib.lib with an old toolset and test.obj with a toolset with this change and manually tried out every combination). Building a static library with the new toolset and linking with an old toolset produces a link error (the old
stl_asan.libdoes not contain__declspec(selectany), so clashes with the definition added in__msvc_sanitizer_annotate_container.hpp), but my understanding is that this isn't a supported scenario.